Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gccrs: cleanup our enum type layout to be closer to rustc #3357

Merged
merged 1 commit into from
Jan 10, 2025

Conversation

philberty
Copy link
Member

This changes our enum type layout so for example:

  enum Foo {
      A,
      B,
      C(char),
      D { x: i32, y: i32 },
  }

Used to get layed out like this in gccrs:

  union {
    struct A { int RUST$ENUM$DISR; };
    struct B { int RUST$ENUM$DISR; };
    struct C { int RUST$ENUM$DISR; char __0; };
    struct D { int RUST$ENUM$DISR; i64 x; i64 y; };
  }

This has some issues notably with the constexpr because this is just a giant union it means its not simple to constify what enum variant we are looking at because the discriminant is a mess.

This now gets layed out as:

  struct {
     int RUST$ENUM$DISR;
     union {
         struct A { };
         struct B { };
         struct C { char __0; };
         struct D { i64 x; i64 y; };
     } payload;
  }

This layout is much cleaner and allows for our constexpr to work properly.

gcc/rust/ChangeLog:

* backend/rust-compile-expr.cc (CompileExpr::visit): new layout
* backend/rust-compile-pattern.cc (CompilePatternCheckExpr::visit): likewise
(CompilePatternBindings::visit): likewise
* backend/rust-compile-resolve-path.cc: likewise
* backend/rust-compile-type.cc (TyTyResolveCompile::visit): implement new layout
* rust-gcc.cc (constructor_expression): get rid of useless assert

This changes our enum type layout so for example:

  enum Foo {
      A,
      B,
      C(char),
      D { x: i32, y: i32 },
  }

Used to get layed out like this in gccrs:

  union {
    struct A { int RUST$ENUM$DISR; };
    struct B { int RUST$ENUM$DISR; };
    struct C { int RUST$ENUM$DISR; char __0; };
    struct D { int RUST$ENUM$DISR; i64 x; i64 y; };
  }

This has some issues notably with the constexpr because this is just a
giant union it means its not simple to constify what enum variant we are
looking at because the discriminant is a mess.

This now gets layed out as:

  struct {
     int RUST$ENUM$DISR;
     union {
         struct A { };
         struct B { };
         struct C { char __0; };
         struct D { i64 x; i64 y; };
     } payload;
  }

This layout is much cleaner and allows for our constexpr to work properly.

gcc/rust/ChangeLog:

	* backend/rust-compile-expr.cc (CompileExpr::visit): new layout
	* backend/rust-compile-pattern.cc (CompilePatternCheckExpr::visit): likewise
	(CompilePatternBindings::visit): likewise
	* backend/rust-compile-resolve-path.cc: likewise
	* backend/rust-compile-type.cc (TyTyResolveCompile::visit): implement new layout
	* rust-gcc.cc (constructor_expression): get rid of useless assert

Signed-off-by: Philip Herron <[email protected]>
@philberty
Copy link
Member Author

our gdb output looks much better with this too.

@philberty philberty requested review from CohenArthur and P-E-P and removed request for CohenArthur January 7, 2025 21:05
Copy link
Member

@P-E-P P-E-P left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not familiar enough with all details here to provide an appropriate review but it looks great and I couldn't find anything to say.

I wonder if this would fix one failing test in #3299

HIR::Expr &discrim_expr = variant->get_discriminant ();
tree discrim_expr_node = CompileExpr::Compile (discrim_expr, ctx);
tree folded_discrim_expr = fold_expr (discrim_expr_node);
tree qualifier = folded_discrim_expr;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why we need that variable ? Could we use folded_discrim_expr directly or rename it ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good find will fix

Copy link
Member

@CohenArthur CohenArthur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM I'm happy with that layout

@philberty philberty added this pull request to the merge queue Jan 10, 2025
Merged via the queue into master with commit 806166d Jan 10, 2025
12 checks passed
@philberty philberty deleted the phil/enum-layout branch January 10, 2025 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants